-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ctrl-D #1402
Fix ctrl-D #1402
Conversation
uint32_t newPos = nextChar(l); | ||
uint32_t oldPos = getBufCharPos(l); | ||
uint32_t charSize = newPos - oldPos; | ||
memmove(l->buf + oldPos, l->buf + newPos, l->len - oldPos); | ||
l->len -= charSize; | ||
l->chars--; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anuchak We should have a mechanism to check whether the character to delete is a utf-8 character or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the name of this variable to totalUTF8Chars
and the mechanism is simple, just check if total size of the character > 1.
Also I'm not sure if this variable is even required or not, because I checked DuckDB's codebase and they have no such tracking variable, don't think it is required at all.
tools/shell/linenoise.cpp
Outdated
@@ -570,6 +570,9 @@ uint32_t linenoiseComputeRenderWidth(const char* buf, size_t len) { | |||
} | |||
|
|||
int getBufCharPos(struct linenoiseState* l) { | |||
if (l->chars == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anuchak I don't really understand what this function is doing? Can you add some comments here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function will most likely not be required, I debugged this function and all it is doing is returning the proper position of the cursor from left.
But l->pos
already holds this, that's why in DuckDB's codebase everywhere only l->pos
has been used instead of calling a separate function.
I'm currently fixing this issue.
I've pushed some changes and they should fix the original issue that the user has raised:
So this is working properly now. But we might need to refer to DuckDB because our UTF8 handling still seems to be inadequate. |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #1402 +/- ##
==========================================
- Coverage 92.80% 92.79% -0.01%
==========================================
Files 642 641 -1
Lines 22066 22043 -23
==========================================
- Hits 20478 20455 -23
Misses 1588 1588 see 7 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Related to issue #1391
This patch only fixes the bug related to ctr-d an ascii-character. If the character is utf-8, then the bug still exists.